On Tue, 25 Jun 2024 at 08:18, Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> On Mon, Jun 24, 2024 at 04:19:52PM GMT, Peter Maydell wrote:
> >On Mon, 24 Jun 2024 at 16:11, Stefano Garzarella <sgarz...@redhat.com> wrote:
> >>
> >> CCing Jason.
> >>
> >> On Mon, Jun 24, 2024 at 4:30 PM Xoykie <xoy...@gmail.com> wrote:
> >> >
> >> > The virtio packed virtqueue support patch[1] suggests converting
> >> > endianness by lines:
> >> >
> >> > virtio_tswap16s(vdev, &e->off_wrap);
> >> > virtio_tswap16s(vdev, &e->flags);
> >> >
> >> > Though both of these conversion statements aren't present in the
> >> > latest qemu code here[2]
> >> >
> >> > Is this intentional?
> >>
> >> Good catch!
> >>
> >> It looks like it was removed (maybe by mistake) by commit
> >> d152cdd6f6 ("virtio: use virtio accessor to access packed event")
> >
> >That commit changes from:
> >
> >- address_space_read_cached(cache, off_off, &e->off_wrap,
> >- sizeof(e->off_wrap));
> >- virtio_tswap16s(vdev, &e->off_wrap);
> >
> >which does a byte read of 2 bytes and then swaps the bytes
> >depending on the host endianness and the value of
> >virtio_access_is_big_endian()
> >
> >to this:
> >
> >+ e->off_wrap = virtio_lduw_phys_cached(vdev, cache, off_off);
> >
> >virtio_lduw_phys_cached() is a small function which calls
> >either lduw_be_phys_cached() or lduw_le_phys_cached()
> >depending on the value of virtio_access_is_big_endian().
> >(And lduw_be_phys_cached() and lduw_le_phys_cached() do
> >the right thing for the host-endianness to do a "load
> >a specifically big or little endian 16-bit value".)
> >
> >Which is to say that because we use a load/store function that's
> >explicit about the size of the data type it is accessing, the
> >function itself can handle doing the load as big or little
> >endian, rather than the calling code having to do a manual swap after
> >it has done a load-as-bag-of-bytes. This is generally preferable
> >as it's less error-prone.
>
> Thanks for the details!
>
> So, should we also remove `virtio_tswap16s(vdev, &e->flags);` ?
>
> I mean:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 893a072c9d..2e5e67bdb9 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -323,7 +323,6 @@ static void vring_packed_event_read(VirtIODevice *vdev,
> /* Make sure flags is seen before off_wrap */
> smp_rmb();
> e->off_wrap = virtio_lduw_phys_cached(vdev, cache, off_off);
> - virtio_tswap16s(vdev, &e->flags);
> }
That definitely looks like it's probably not correct...
-- PMM
